Add support for device-code oauth login #5244
Conversation
laurazard
changed the title
laurazard
force-pushed
the
auth-device-flow-2
branch
4 times, most recently
from
5368212
to
7a1da8e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5244 +/- ##
==========================================
+ Coverage 61.45% 61.65% +0.19%
==========================================
Files 299 304 +5
Lines 20857 21180 +323
==========================================
+ Hits 12818 13058 +240
- Misses 7124 7192 +68
- Partials 915 930 +15 |
laurazard
force-pushed
the
auth-device-flow-2
branch
9 times, most recently
from
dd7d670
to
4674044
Compare
laurazard
force-pushed
the
auth-device-flow-2
branch
2 times, most recently
from
b392301
to
d720959
Compare
Screen.Recording.2024-07-09.at.11.16.43.mov |
laurazard
changed the title
laurazard
force-pushed
the
auth-device-flow-2
branch
3 times, most recently
from
0be6989
to
f937eeb
Compare
laurazard
force-pushed
the
auth-device-flow-2
branch
3 times, most recently
from
0d948ee
to
34ee692
Compare
|
Just tested that vendoring the changes into buildkit/buildx without any additional changes allows pulling images from private repos while building. |
There was a problem hiding this comment.
Left some blurbs; also playing with some options in those areas.
Overall (from a user perspective) it all works pretty neat. Some notes I had while playing;
I think we may be missing some normalization of registry-names still (likely not something new); for example;
docker login registry-1.docker.io
Username:But docker.io works;
docker login docker.io
You will be signed in using a web-based login.
To sign in with credentials on the command line, use 'docker login -u <username>'
...Probably not problematic, but I noticed that a mix of "logout" using the old CLI, and login using the "new" CLI showed this;
# this is the old CLI:
docker logout
Removing login credentials for https://index.docker.io/v1/
# login with the new CLI:
./build/docker login
Authenticating with existing credentials...
You will be signed in using a web-based login.
To sign in with credentials on the command line, use 'docker login -u <username>'
...Notice that it prints Authenticating with existing credentials...; I think what happens is that the old CLI only removes credentials it knows about, but then leaves the new ones. The new CLI (probably) sees that there's credentials, but then they don't work, so it continues the new path to interactively authenticate. Definitely not problematic AFAICS, just thinking if we can detect that situation.
Also had a try what happens if hub is not accessible when logging out / in (making sure we don't fully hang);
docker login docker.io
# You will be signed in using a web-based login.
# ...
# block login.docker.com
echo '127.0.0.1 login.docker.com' >> /etc/hosts
docker logout
Removing login credentials for https://index.docker.io/v1/
docker login
login failed: Post "https://login.docker.com/oauth/device/code": dial tcp 127.0.0.1:443: connect: connection refusedSlightly wondering if we should print a message if logout failed; we continue removing the credentials from the store, but RevokeToken would've failed; is that something important to log (i.e., if the token was found by someone, it could still be valid?).
| return credentials.NewFileStore(configFile) | ||
| return credentials.NewOAuthStore(credsStore, manager.NewManager()) |
There was a problem hiding this comment.
Thinking now; instead of oauthStore (oauthStore.Get() etc) checking for if serverAddress != defaultRegistry {, should that be handled here instead; i.e. along these lines;
if serverAddress == defaultRegistry {
credsStore = credentials.NewOAuthStore(credsStore, manager.NewManager())
}
return credsStoreAlternatively, we could have a way to set some helper to do this; that way we can keep the implementation out of here, and have some way to set "how to construct a helper")
if configFile.credsHelperProvider != nil {
return configFile.credsHelperProvider(registryHostname, credsStore)
}I wish we hadn't conflated the ConfigFile type (for serialising) with the ConfigFile functionality (saving, loading, manipulating) that much though. We need to look if we can get out of that at some point.
There was a problem hiding this comment.
Hmm, I find it easier to reason about if we have a single point managing both. Otherwise, if someone does GetCredentialsStore("some-other-registry").GetAll() they're going to get the access-token and refresh-token specific keys that we shouldn't leak out/callers shouldn't concern themselves with. There might be other things that the wrapper does that should be abstracted away by the wrapper.
|
Oh! Erm, this one I forgot to post; something I didn't consider in my changes I guess; we store multiple times now, and each of them prints a warning 🙈 docker login
You will be signed in using a web-based login.
To sign in with credentials on the command line, use 'docker login -u <username>'
Your one-time device confirmation code is: XXXXXX
Press ENTER to open the browser.
Or open the URL manually: https://login.docker.com/activate
Waiting for authentication in the browser...
WARNING! Your credentials are stored unencrypted in '/root/.docker/config.json'.
Configure a credential helper to remove this warning. See
https://docs.docker.com/go/credential-store/
WARNING! Your credentials are stored unencrypted in '/root/.docker/config.json'.
Configure a credential helper to remove this warning. See
https://docs.docker.com/go/credential-store/
Login SucceededThis one was slightly more surprising; after Having signed in, running docker login
WARNING! Your credentials are stored unencrypted in '/root/.docker/config.json'.
Configure a credential helper to remove this warning. See
https://docs.docker.com/go/credential-store/
WARNING! Your credentials are stored unencrypted in '/root/.docker/config.json'.
Configure a credential helper to remove this warning. See
https://docs.docker.com/go/credential-store/
Authenticating with existing credentials...
WARNING! Your credentials are stored unencrypted in '/root/.docker/config.json'.
Configure a credential helper to remove this warning. See
https://docs.docker.com/go/credential-store/
Login Succeeded |
laurazard
force-pushed
the
auth-device-flow-2
branch
2 times, most recently
from
9ef2396
to
78a84c1
Compare
|
Re: the warning now printing multiple times, I'm wondering how to address that. We store twice (for both keys, now) and on login, since we refresh the token and store the new one. |
This is a good question – it kind of sucks in this case since we'd be removing the token, effectively making it so that the user no longer has the token to revoke it. Definitely should log a message in this case. |
| err = o.manager.Logout(ctx, refreshTokenAuth.Password) | ||
| if err != nil { | ||
| // todo(laurazard): actual message here | ||
| fmt.Fprint(os.Stderr, "Failed to revoke refresh token with tenant.. Credentials will still be erased.") |
There was a problem hiding this comment.
Ideas for a better message here?
There was a problem hiding this comment.
@dvdksn thoughts on this one?
There was a problem hiding this comment.
What is the user action that leads here? docker logout myregistry.example.com?
There was a problem hiding this comment.
This only applies to logging out from the official registry, so either docker logout or docker logout index.docker.io, docker logout registry-1.docker.io
|
@thaJeztah also wondering about how we'll fix the multiple file_store warnings, now that the warnings live in the store 😅 Ideas? I did a hacky solution in be4ecc3, but open to alternatives. |
laurazard
force-pushed
the
auth-device-flow-2
branch
from
be4ecc3
to
6a838e2
Compare
| err = o.manager.Logout(ctx, refreshTokenAuth.Password) | ||
| if err != nil { | ||
| // todo(laurazard): actual message here | ||
| fmt.Fprint(os.Stderr, "Failed to revoke refresh token with tenant.. Credentials will still be erased.") |
There was a problem hiding this comment.
nit:
| fmt.Fprint(os.Stderr, "Failed to revoke refresh token with tenant.. Credentials will still be erased.") | |
| _, _ = fmt.Fprint(os.Stderr, "Failed to revoke refresh token with tenant.. Credentials will still be erased.") |
(to be consistent with other Fprints)
There was a problem hiding this comment.
What about making this a warning? I don't recall if we have a helper function for styling, but something like:
| fmt.Fprint(os.Stderr, "Failed to revoke refresh token with tenant.. Credentials will still be erased.") | |
| _, _ = fmt.Fprint(os.Stderr, "Warning: You have been logged out successfully, but there was a failure to revoke the OAuth refresh token with the tenant.") |
The only problem with the above is that it doesn't really say anything about manually addressing that failure. A more complete, but unattractively longer, version could be:
| fmt.Fprint(os.Stderr, "Failed to revoke refresh token with tenant.. Credentials will still be erased.") | |
| _, _ = fmt.Fprint(os.Stderr, "Warning: You have been logged out successfully, but there was a failure to revoke the OAuth refresh token with the tenant. You may want to manually revoke the token through your OAuth provider's settings.") |
There was a problem hiding this comment.
Wasn't aware of the prior art 👍🏻 In that case s/You have been logged out successfully/Credentials erased successfully/ for consistency.
laurazard
force-pushed
the
auth-device-flow-2
branch
3 times, most recently
from
b884d32
to
1b798cf
Compare
This commit adds support for the oauth [device-code](https://auth0.com/docs/get-started/authentication-and-authorization-flow/device-authorization-flow) login flow when authenticating against the official registry. This is achieved by adding `cli/internal/oauth`, which contains code to manage interacting with the Docker OAuth tenant (`login.docker.com`), including launching the device-code flow, refreshing access using the refresh-token, and logging out. The `OAuthManager` introduced here is also made available through the `command.Cli` interface method `OAuthManager()`. In order to manage storage and retrieval of the oauth credentials, as well as ensuring that the access token retrieved from the credentials store is valid when accessed, this commit also introduces a new credential store wrapper, `OAuthStore`, which defers storage to an underlying store (such as the file or native/keychain/pass/wincred store) and gets a new access token using the refresh token when accessed. Signed-off-by: Laura Brehm <[email protected]>
Signed-off-by: Laura Brehm <[email protected]>
Signed-off-by: Laura Brehm <[email protected]>
Signed-off-by: Laura Brehm <[email protected]>
Signed-off-by: Laura Brehm <[email protected]>
Signed-off-by: Laura Brehm <[email protected]>
Signed-off-by: Laura Brehm <[email protected]>
laurazard
force-pushed
the
auth-device-flow-2
branch
from
1b798cf
to
0bac5a7
Compare
|
Closed in favor of #5344 |
Signed-off-by: Laura Brehm [email protected]
- What I did
This PR adds support for the oauth device-code login flow when authenticating against the official registry.
- How I did it
Added
cli/internal/oauth, which contains code to manage interacting with the Docker OAuth tenant (login.docker.com), including launching the device-code flow, refreshing access using the refresh-token, and logging out.In order to manage storage and retrieval of the oauth credentials, as well as ensuring that the access token retrieved from the credentials store is valid when accessed, this PR also introduces a new credential store wrapper,
OAuthStore, which defers storage to an underlying store (such as the file or native/keychain/pass/wincred store) and gets a new access token using the refresh token when accessed.Also included an (imo overdue 😅) refactor of
cli/command/registry/login.go.- How to verify it
Try to login, do things that require authentication, and logout!
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)